Skip to content

fix(vllm): tell the reasoning parser whether thinking was enabled - #11791

Open
pos-ei-don wants to merge 1 commit into
mudler:masterfrom
pos-ei-don:vllm-reasoning-thinking-state
Open

fix(vllm): tell the reasoning parser whether thinking was enabled#11791
pos-ei-don wants to merge 1 commit into
mudler:masterfrom
pos-ei-don:vllm-reasoning-thinking-state

Conversation

@pos-ei-don

Copy link
Copy Markdown
Contributor

Description

vLLM's engine-based reasoning parsers derive their initial state from the chat template
kwargs. Qwen3Parser:

chat_kwargs = kwargs.get("chat_template_kwargs", {}) or {}
self.thinking_enabled = chat_kwargs.get("enable_thinking", True)

Constructed as ReasoningParser(tokenizer) the flag defaults to True, so the parser starts
in the REASONING state. A completion produced with thinking disabled contains no tags at all,
and every reasoning parser shape then reports the whole answer as reasoning:

  • engine-based parsers classify it by initial state;
  • BaseThinkingReasoningParser hits its documented "may not generate start token" fallback
    and returns (model_output, None).

Either way content = c if c is not None else generated_text turns that into a duplicate: a
Qwen3 model answering 391 with thinking off comes back as reasoning_content="391" and
content="391".

Measured against Qwen3.5-MoE on vLLM 0.28, non-streaming:

before   thinking on   reasoning=202   content="391"
         thinking off  reasoning="391" content="391"   <- duplicated
after    thinking on   reasoning=192   content="391"
         thinking off  reasoning=""    content="391"

The fix forwards the kwargs the prompt was rendered with, which is what vLLM's own OpenAI
server does; parsers that do not accept the argument keep the plain constructor.

Notes for Reviewers

The parser class matters here. On vLLM 0.28 the qwen3 name resolves to
Qwen3ParserReasoningAdapter, an engine-based adapter with no start_token — not to
BaseThinkingReasoningParser. A fix written against the latter is inert against the shipped
parser, which is why this one sets the state through the constructor rather than around it.

_split_reasoning() covers the older parser shape, which has no initial state to set. It
only reclassifies when the parser exposes a start/end token pair and neither the
completion nor the prompt ever opened a reasoning block. Truncated reasoning (block open, end
token never arrived) stays reasoning, and parsers without that token pair are left untouched.

Worth noting for anyone reading this alongside reasoning_effort: vLLM forwards the value
into template kwargs but filters it out when the template does not declare it. For a template
that only knows enable_thinking, thinking is binary and reasoning_effort has no effect
beyond on/off — this PR does not change that.

Signed commits

  • Yes, I signed my commits.
  • Documentation updated (docs/content/) for user-facing changes, or not applicable

vLLM's engine-based reasoning parsers derive their initial state from the
chat template kwargs. Qwen3Parser:

    chat_kwargs = kwargs.get("chat_template_kwargs", {}) or {}
    self.thinking_enabled = chat_kwargs.get("enable_thinking", True)

Constructed as ReasoningParser(tokenizer) the flag defaults to True, so the
parser starts in the REASONING state. A completion produced with thinking
disabled contains no tags at all, and every reasoning parser shape then
reports the whole answer as reasoning:

  - engine-based parsers classify it by initial state;
  - BaseThinkingReasoningParser hits its documented "may not generate start
    token" fallback and returns (model_output, None).

Either way `content = c if c is not None else generated_text` turns that
into a duplicate: a Qwen3 model answering "391" with thinking off comes back
as reasoning_content="391" AND content="391".

Measured against Qwen3.5-MoE on vLLM 0.28, non-streaming:

    before   thinking on   reasoning=202  content="391"
             thinking off  reasoning="391" content="391"   <- duplicated
    after    thinking on   reasoning=192  content="391"
             thinking off  reasoning=""    content="391"

Forward the kwargs the prompt was rendered with, which is what vLLM's own
OpenAI server does; parsers that do not accept the argument keep the plain
constructor.

_split_reasoning() covers the older parser shape, which has no initial state
to set. It only reclassifies when the parser exposes a start/end token pair
and neither the completion nor the prompt ever opened a reasoning block.
Truncated reasoning (block open, end token never arrived) stays reasoning,
and parsers without that token pair are left untouched.

Signed-off-by: pos-ei-don <1822533+pos-ei-don@users.noreply.github.com>

@localai-org-maint-bot localai-org-maint-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to merge from my review. The parser now receives the same template kwargs used to render the prompt, while the fallback preserves older parser compatibility; the split logic also distinguishes plain answers from truncated reasoning. Regression coverage, Python syntax checks, and diff checks look clean. @mudler

@localai-org-maint-bot

Copy link
Copy Markdown
Collaborator

Review pass. The _split_reasoning half is well argued and well tested. Two smaller things.

What is right: BaseThinkingReasoningParser returning the whole completion as reasoning with None content, combined with content = c if c is not None else generated_text, really does duplicate a thinking-disabled answer into both fields. Using the prompt to distinguish "block was open" from "block never opened" is the right discriminator, and the tests cover it.

Nit: _new_reasoning_parser(template_kwargs) forwards too much.

At vllm/backend.py:655-666 that dict also contains tokenize, add_generation_prompt and possibly a parsed tools list alongside enable_thinking. vLLM's own server forwards only the request's chat template kwargs. Suggest passing {"enable_thinking": ...} when present rather than the whole render-options dict.

Coverage gap worth reflecting in the title.

template_kwargs is only populated inside the not request.Prompt and request.UseTokenizerTemplate and request.Messages branch. When LocalAI renders the prompt in Go and sends request.Prompt, the dict stays {}, an engine-based Qwen3Parser still defaults to enable_thinking=True, and _split_reasoning cannot rescue it because those adapters expose no start_token/end_token and fall into the if not start passthrough. So the described bug survives on that path. Not a regression, but the title overstates the coverage.

Behaviour change worth knowing: when the end token is present and the parser returns content=None, content is now "" rather than the raw text. Tested and defensible, but it is a change for anyone relying on the old duplication.

Two process notes: backend/python/vllm/test.py does not run in CI (no tests-vllm job; it is commented out in test-extra.yml), so the green check says nothing about the changed code. And this PR conflicts with #11738 in the same _predict template block and with #11786 in the same test.py region, so whichever lands first will force a rebase on the others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants